[Opt](bitmap) Optimize bitmap op count rewrites#65483
Conversation
linrrzqqq
requested review from
924060929,
Gabriel39,
englefly,
morrySnow,
starocean999 and
yiguolei
as code owners
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I found one compatibility issue that should be fixed before merge.
Critical checkpoints: the PR's goal is clear and the bitmap-count rewrite/fast paths are covered by focused unit and regression additions, but the FE/BE compatibility checkpoint is not satisfied for rolling upgrades. The change is otherwise focused; I did not find concurrency, lifecycle, persistence, observability, or transaction risks in the changed paths. Parallel execution paths for query, load, insert, stream load, and BE constant folding were checked for query-option propagation. Tests were reviewed statically but not run because this checkout is missing .worktree_initialized and thirdparty/installed/bin/protoc.
Subagent conclusions: optimizer-rewrite reported no additional candidates. tests-session-config reported the same rolling-upgrade nullability issue as TSC-1, merged as duplicate of MRG-001. Convergence round 1 ended with both live subagents replying NO_NEW_VALUABLE_FINDINGS for the final ledger/comment set.
User focus: no additional user-provided review focus was supplied.
There was a problem hiding this comment.
Anchor repair for the same compatibility issue: the previous review summary remains applicable, and the misplaced left-side inline comment was deleted. This review resubmits the inline comment on the added right-side line.
Critical checkpoints: the PR's goal is clear and the bitmap-count rewrite/fast paths are covered by focused unit and regression additions, but the FE/BE compatibility checkpoint is not satisfied for rolling upgrades. The change is otherwise focused; I did not find concurrency, lifecycle, persistence, observability, or transaction risks in the changed paths. Parallel execution paths for query, load, insert, stream load, and BE constant folding were checked for query-option propagation. Tests were reviewed statically but not run because this checkout is missing .worktree_initialized and thirdparty/installed/bin/protoc.
Subagent conclusions: optimizer-rewrite reported no additional candidates. tests-session-config reported the same rolling-upgrade nullability issue as TSC-1, merged as duplicate of MRG-001. Convergence round 1 ended with both live subagents replying NO_NEW_VALUABLE_FINDINGS for the final ledger/comment set.
User focus: no additional user-provided review focus was supplied.
TPC-H: Total hot run time: 29266 ms |
TPC-DS: Total hot run time: 179900 ms |
ClickBench: Total hot run time: 25.5 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
linrrzqqq
force-pushed
the
bitmap-op-count-v2
branch
from
90a0650 to
2848f59
Compare
linrrzqqq
force-pushed
the
bitmap-op-count-v2
branch
from
2848f59 to
ae4aa4e
Compare
|
/review |
There was a problem hiding this comment.
Review result: no additional inline findings.
The rolling-upgrade compatibility defect already reported in #65483 (comment) remains unresolved and blocking, so I did not duplicate it. In particular, a new FE can advertise non-null BIGINT while an old BE ignores the optional flag and binds the legacy Nullable(BIGINT) implementation for nullable bitmap arguments. Same-version execution and the optimization itself otherwise look correct.
Critical checkpoints:
- Goal and proof: the rewrite reaches the intended direct bitmap cardinality implementations, preserves AND/OR/XOR/AND-NOT row and NULL semantics, and has FE rewrite tests, BE function/cardinality tests, and regression coverage. The rolling-upgrade compatibility part of the end-to-end goal is still incomplete because of the existing blocker.
- Scope: the production, protocol, alias, and test changes are focused on bitmap op-count selection and execution; no unrelated change was found.
- Concurrency: no new thread, shared mutable state, lock, or lock-order dependency is introduced. Function registration is initialized once and then read-only; execution state is local to each block/function call.
- Lifecycle/static initialization: no new non-trivial lifecycle, ownership cycle, cross-TU static dependency, or shutdown behavior is introduced. Raw input-column pointers in the fast path remain owned by the input block for the call duration.
- Configuration: no configuration item or dynamic-reload contract is added.
- Compatibility: old-FE/new-BE stays on the legacy path and inspected new-FE/new-BE paths select
_v2; new-FE/old-BE with nullable arguments remains incompatible as described in the existing thread. There is no backend capability, scheduler, thrift, orbe_exec_versiongate that prevents it. - Parallel paths: normal query, point query, insert, broker/load, Nereids stream load, forwarding/replanning, and BE constant-fold option construction were traced. No additional missing propagation path was found.
- Conditional checks: the new
__isset/attribute checks are simple path selection, but they do not solve the known mixed-version case. No separate questionable conditional or silently ignored error path was found. - Test coverage: the changed FE, BE, and regression tests cover rewrite shape, aliases, nullable vectors, legacy/new return types, and bitmap representation pairs. Constant nullable permutations are not separately enumerated, but the const/vector, const-null, zero-row, lifetime, and result-shape code paths were traced without finding a defect.
- Test results: the changed
.outentries correspond to the added deterministic scalar queries. No local build or tests were run under this review-runner contract;.worktree_initialized,thirdparty/installed, andthirdparty/installed/bin/protocare absent. CI checkstyle and format checks pass, while the observed macOS BE-UT job stopped before build/test because it used JDK 25 instead of the required JDK 17. - Observability: no new distributed state transition or independently diagnosable operational path requires logs or metrics; existing preparation errors surface the known compatibility failure.
- Transactions and persistence: no EditLog, replay, transaction, master-failover, or persisted-data-format behavior is changed. Stored function/view spellings reparse through the surviving aliases.
- Data writes: no storage mutation, atomicity, crash-recovery, or write-concurrency path is introduced.
- FE-to-BE variables: the new optional query option is set on all inspected same-version sending/construction paths, including constant folding and point-query flows. Its lack of an old-BE capability gate is the existing blocker.
- Performance and memory: the binary fast paths avoid materializing intermediate bitmaps, reuse read-only input storage safely, preserve result-column shape, and show no new redundant scan, allocation, or memory-accounting problem.
- Other issues: alias canonicalization, visitor removal, error/volatile-child evaluation, cloud/shared-nothing applicability, and function-name/return-type consistency were checked; no additional substantiated defect remains.
User focus: no additional review focus was provided.
Review completion: complete. Three normal reviewers, three risk-focused reviewers, and a second same-ledger convergence pass all returned NO_NEW_VALUABLE_FINDINGS; the only substantiated issue is the existing unresolved blocker linked above.
|
run buildall |
FE Regression Coverage ReportIncrement line coverage |
2 participants
Problem Summary:
Background
bitmap_count(bitmap_op(...))currently materializes an intermediate bitmap before counting. For patterns such asbitmap_count(bitmap_and(...)),bitmap_count(bitmap_or(...)),bitmap_count(bitmap_xor(...)), andbitmap_count(bitmap_and_not(...)), Doris already has correspondingbitmap_*_countfunctions that can compute the cardinality directly.Also,
bitmap_and_count,bitmap_or_count,bitmap_xor_count, andbitmap_and_not_countare semantically always not null, but their previous metadata could be marked nullable when bitmap arguments were nullable.Changes
Add Nereids expression rewrite:
bitmap_count(bitmap_and(...))->bitmap_and_count(...)bitmap_count(bitmap_or(...))->bitmap_or_count(...)bitmap_count(bitmap_xor(...))->bitmap_xor_count(...)bitmap_count(bitmap_not(...))/bitmap_and_not(...)->bitmap_and_not_count(...)Mark bitmap op count scalar functions as always not nullable in Nereids.
Add BE v2 implementations for bitmap op count return type inference:
bitmap_and_count_v2bitmap_or_count_v2bitmap_xor_count_v2bitmap_and_not_count_v2Add
new_version_bitmap_op_countquery option to select the v2 BE implementations.Optimize binary bitmap count execution paths to avoid intermediate bitmap materialization.
Fix
BitmapValue::or_cardinalityfor SET/SET input.Merge
bitmap_and_not/bitmap_andnotaliases intobitmap_not.